[GH-1996] Create object of S2Geography, and implement Point/Polyline/PolygonGeography with its encoder/decoder by ZhuochengShang · Pull Request #1992 · apache/sedona

ZhuochengShang · 2025-06-20T16:17:08Z

Did you read the Contributor Guide?

Yes, I have read the Contributor Rules and Contributor Development Guide

Is this PR related to a ticket?

Yes, and the PR name follows the format [GH-XXX] my subject.

What changes were proposed in this PR?

The first step of porting https://github.com/paleolimbot/s2geography, ported S2Geography base class and 3 sub types.

How was this patch tested?

Added unit tests.

Did this PR include necessary documentation updates?

No, this PR does not affect any public API so no need to change the documentation.

…coder/decoder

jiayuasu · 2025-06-20T16:45:16Z

@ZhuochengShang great work. Please create a GitHub issue and link the PR to that issue using [GH-XXXX]

jiayuasu · 2025-06-20T23:22:53Z

Great work @ZhuochengShang . It is great to have a working PR that shows the progress. But when it is ready, we'd like to break it into a sequence of small PRs. This will make the review process easier.

jiayuasu · 2025-06-20T23:23:27Z

Please also update the title of this PR to [GH-XXXX] that links to the GitHub issue.

paleolimbot · 2025-06-23T15:29:09Z

Thank you for this work so far! Just a note that if you (or Kristin or Jia) run across anything that doesn't make sense or is sub-optimial in the C++ version of S2Geography to let me know!

- Import org.datasyslab s2-geometry-library - Clean up S2Geography abstract design - Update Encode/Decode inside each kind of geography

- Adding back EncodeTagged in S2Geography - Let each geography type calls its own encode / decode function - Change to use Kyro UnsafeInput and UnsafeOutput

Kontinuation

The overall structure looks good. There are some additional comments mostly for the decoding part.

I also suggest that we add more comprehensive tests for encoding and decoding of geography objects. For instance, includeCovering option has not been tested for now. Adding a utility test function for round-trip encoding/decoding any geography objects and validate their equality would help writing such tests a lot.

Kontinuation · 2025-06-26T03:35:50Z

+public class PolylineGeography extends S2Geography {
+  private static final Logger logger = Logger.getLogger(PolylineGeography.class.getName());
+
+  private final List<S2Polyline> polylines;


We define PolylineGeography to contain a list of S2Polylines, which is different from the C++ version. I think this is a reasonable change, as it makes distinguishing between POLYGON and MULTIPOLYGON much easier as long as we ensure that each S2Polyline only contains one polygon. WDYT @paleolimbot

I believe that Polyline in C++ is also a std::vector<S2Polyline>, but you're correct that the polygon is defined this way.

Paul (PostGIS) complained about this too, mostly because some multipolygons don't roundtrip through WKB/T/JTS. I've personally found it easier to keep the S2-centric structure since a lot of the time internally it doesn't matter. Either way is fine!

Copilot

Pull Request Overview

Adds support for S2Geography objects (Point, Polyline, Polygon) with Kryo‐based encoding/decoding and corresponding round‐trip tests, and updates the S2 library dependency.

Introduce S2Geography base class and PointGeography, PolylineGeography, PolygonGeography subclasses with encode/decode logic
Add TestHelper utility and unit tests for each geography type (round‐trip and covering assertions)
Update pom.xml to use the forked org.datasyslab:s2-geometry-library and shade Google’s S2 package

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
common/src/main/java/org/apache/sedona/common/S2Geography/S2Geography.java	Base abstract class and encoding/decoding machinery
common/src/main/java/org/apache/sedona/common/S2Geography/PointGeography.java	Implements point encoding/decoding and optimized compact path
common/src/main/java/org/apache/sedona/common/S2Geography/PolylineGeography.java	Implements polyline path handling and (de)serialization
common/src/main/java/org/apache/sedona/common/S2Geography/PolygonGeography.java	Implements single‐loop polygon handling and (de)serialization
common/src/test/java/org/apache/sedona/common/S2Geography/TestHelper.java	Utility for verifying round‐trip correctness and coverings
common/src/test/java/org/apache/sedona/common/S2Geography/PointGeographyTest.java	Point geography unit tests
common/src/test/java/org/apache/sedona/common/S2Geography/PolylineGeographyTest.java	Polyline geography unit tests
common/src/test/java/org/apache/sedona/common/S2Geography/PolygonGeographyTest.java	Polygon geography unit tests
common/pom.xml	Swap in the forked S2 library and add shading for Google S2

Comments suppressed due to low confidence (1)

common/src/test/java/org/apache/sedona/common/S2Geography/TestHelper.java:83

[nitpick] The error message uses a hard-coded index '1' for both polygon and loop, but you are checking loop zero. Consider using the actual loop index (e.g., '0') or dynamically inserting the index to avoid confusion.

          "Loop count mismatch in polygon[" + 1 + "]", pgOrig.numLoops(), pgDec.numLoops());

jiayuasu · 2025-06-30T18:21:16Z

What is the status of this PR?

Kontinuation

There are some minor issues related to the revised PolygonGeography. I think this PR is close to completion.

Kontinuation

Now it looks good to me. Thank you for your great work @ZhuochengShang

paleolimbot · 2025-07-01T15:20:55Z

Awesome!

…yline/PolygonGeography with its encoder/decoder (apache#1992) * Create object of S2Geography, and implement PoinGeography with its encoder/decoder * Add POLYLINE implementation on S2Geography * Add POLYGON implements on S2Geography * Match coding style * "Apply Spotless formatting to PolylineGeographyTest" * Redesign of S2Geography - Import org.datasyslab s2-geometry-library - Clean up S2Geography abstract design - Update Encode/Decode inside each kind of geography * clean up unnecessary files in current branch * Refine design of EncodeTagged in S2Geography - Adding back EncodeTagged in S2Geography - Let each geography type calls its own encode / decode function - Change to use Kyro UnsafeInput and UnsafeOutput * Modify encoder() and add new test cases * clean up code of encode and clarify comments * Update POLYGON to only take one polygon * Remove S2Regionwrapper & S2Shapewrapper * clean up minor issue * resolve minor issue with PolygonGeography

Create object of S2Geography, and implement PoinGeography with its en…

a9796da

…coder/decoder

ZhuochengShang requested a review from jiayuasu as a code owner June 20, 2025 16:17

github-actions Bot added the sedona-common label Jun 20, 2025

ZhuochengShang requested a review from Kontinuation June 20, 2025 16:17

Merge branch 'apache:master' into zshang

bf9c139

ZhuochengShang mentioned this pull request Jun 20, 2025

[GH-1996] Implement S2Geography support on Apache Sedona #1996

Closed

ZhuochengShang added 4 commits June 20, 2025 14:21

Add POLYLINE implementation on S2Geography

49a6484

Add POLYGON implements on S2Geography

42a703d

Match coding style

02993ed

"Apply Spotless formatting to PolylineGeographyTest"

32df1c3

ZhuochengShang changed the title ~~Create object of S2Geography, and implement PoinGeography with its en…~~ [GH-1996]Create object of S2Geography, and implement PoinGeography with its en… Jun 20, 2025

Kontinuation reviewed Jun 23, 2025

View reviewed changes

ZhuochengShang added 2 commits June 23, 2025 17:08

Redesign of S2Geography

479f611

- Import org.datasyslab s2-geometry-library - Clean up S2Geography abstract design - Update Encode/Decode inside each kind of geography

clean up unnecessary files in current branch

9be05ea

Kontinuation reviewed Jun 24, 2025

View reviewed changes

Comment thread common/src/main/java/org/apache/sedona/common/S2Geography/S2Geography.java Outdated

Comment thread common/src/main/java/org/apache/sedona/common/S2Geography/PolylineGeography.java Outdated

Refine design of EncodeTagged in S2Geography

cf2423c

- Adding back EncodeTagged in S2Geography - Let each geography type calls its own encode / decode function - Change to use Kyro UnsafeInput and UnsafeOutput

Kontinuation reviewed Jun 25, 2025

View reviewed changes

Modify encoder() and add new test cases

44392ce

Kontinuation reviewed Jun 26, 2025

View reviewed changes

jiayuasu reviewed Jun 26, 2025

View reviewed changes

Comment thread common/src/test/java/org/apache/sedona/common/S2Geography/PointGeographyTest.java Outdated

ZhuochengShang added 2 commits June 26, 2025 09:44

clean up code of encode and clarify comments

3eb3626

Update POLYGON to only take one polygon

1aec27f

Kontinuation reviewed Jun 27, 2025

View reviewed changes

Comment thread common/src/main/java/org/apache/sedona/common/S2Geography/PolygonGeography.java Outdated

Comment thread common/src/main/java/org/apache/sedona/common/S2Geography/PolygonGeography.java Outdated

Remove S2Regionwrapper & S2Shapewrapper

a4ceb15

jiayuasu requested a review from Copilot June 27, 2025 20:00

Copilot AI reviewed Jun 27, 2025

View reviewed changes

clean up minor issue

511220c

Kontinuation reviewed Jul 1, 2025

View reviewed changes

resolve minor issue with PolygonGeography

42125f2

Kontinuation approved these changes Jul 1, 2025

View reviewed changes

Kontinuation changed the title ~~[GH-1996]Create object of S2Geography, and implement PoinGeography with its en…~~ [GH-1996] Create object of S2Geography, and implement PoinGeography with its encoder/decoder Jul 1, 2025

Kontinuation changed the title ~~[GH-1996] Create object of S2Geography, and implement PoinGeography with its encoder/decoder~~ [GH-1996] Create object of S2Geography, and implement Point/Polyline/PolygonGeography with its encoder/decoder Jul 1, 2025

jiayuasu approved these changes Jul 1, 2025

View reviewed changes

jiayuasu added this to the sedona-1.8.0 milestone Jul 1, 2025

jiayuasu merged commit cf01102 into apache:master Jul 1, 2025
35 checks passed

jiayuasu added improvement affect public APIs labels Jul 1, 2025

ZhuochengShang deleted the zshang branch July 2, 2025 20:35

Conversation

ZhuochengShang commented Jun 20, 2025 • edited by Kontinuation Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Did you read the Contributor Guide?

Is this PR related to a ticket?

What changes were proposed in this PR?

How was this patch tested?

Did this PR include necessary documentation updates?

Uh oh!

jiayuasu commented Jun 20, 2025

Uh oh!

jiayuasu commented Jun 20, 2025

Uh oh!

jiayuasu commented Jun 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paleolimbot commented Jun 23, 2025

Uh oh!

Uh oh!

Uh oh!

Kontinuation left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kontinuation Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

paleolimbot Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jiayuasu commented Jun 30, 2025

Uh oh!

Kontinuation left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kontinuation left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paleolimbot commented Jul 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ZhuochengShang commented Jun 20, 2025 •

edited by Kontinuation

Loading